Skip to content

Conversation

nitrofski
Copy link
Contributor

@nitrofski nitrofski commented Sep 22, 2025

Which problem is this PR solving?

At the moment, GraphQL "resolve" spans (spans generated for each resolver/sub-resolver in a GraphQL request) are generated in a tree structure that reflects the GraphQL resolution tree. This sounds great in theory, but in practice it generates this kind of traces, where child spans start when their parent spans end.

 v root span                             | ----------------------------------------
 |  v resolve thing                      | ----------
 |  |  v resolve thing.field1            |           ----------
 |  |  \-    resolve thing.field1.nested |                     --------------------
 \- \-   resolve thing.field2            |           --------------------

Although it may look fine at a glance, it has issues:

  • Collapsing a span with child spans in a trace viewer (maybe some of them handle this better?) results in some amount of time seemingly unaccounted for in the root span. E.g.:

     v root span                             | ----------------------------------------
     |  v resolve thing                      | ----------
     |  |  > resolve thing.field1            |           ----------
     \- \-   resolve thing.field2            |           --------------------
    
     v root span                             | ----------------------------------------
     \- > resolve thing                      | ----------
    
  • Critical path in viewers like Jaeger does not generate properly

Short description of the changes

The solution is to never have a span end before all its child spans have ended. I can imagine two ways to achieve this:

  1. Add an extra layer of spans for the tree structure, and put the "resolve" spans under their tree structure node.

     v root span                    | ----------------------------------------
     |  v thing                     | ----------------------------------------
     |  |    resolve                | ----------
     |  |  v thing.field1           |           ------------------------------
     |  |  |    resolve             |           ----------
     |  |  |  v thing.field1.nested |                     --------------------
     |  |  \- \-   resolve          |                     --------------------
     |  |  v thing.field2           |           --------------------
     \- \- \-   resolve             |           --------------------
    
  2. Put all "resolve" spans under the same parent.

    v root span                      | ----------------------------------------
    |    resolve thing               | ----------
    |    resolve thing.field1        |           ----------
    |    resolve thing.field2        |           --------------------
    \-   resolve thing.field1.nested |                     --------------------
    

This PR implements option 2. for simplicity, although I am open to work toward option 1. if preferred.

@nitrofski
Copy link
Contributor Author

@david-luna This is the other change I would like to contribute. I'm open to discussion since this isn't as black-and-white as the bug fix we just merged.

Copy link

codecov bot commented Sep 22, 2025

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.67%. Comparing base (b11e61e) to head (f8ec05c).

Files with missing lines Patch % Lines
packages/instrumentation-graphql/src/utils.ts 95.65% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3085   +/-   ##
=======================================
  Coverage   92.67%   92.67%           
=======================================
  Files         237      237           
  Lines       11247    11251    +4     
  Branches     2334     2336    +2     
=======================================
+ Hits        10423    10427    +4     
  Misses        824      824           
Flag Coverage Δ
auto-instrumentations-node 98.24% <ø> (ø)
instrumentation-graphql 94.15% <95.65%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ages/instrumentation-graphql/src/internal-types.ts 100.00% <ø> (ø)
packages/instrumentation-graphql/src/utils.ts 93.57% <95.65%> (+0.12%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nitrofski
Copy link
Contributor Author

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Hmm. This is bothersome. The line missing coverage was subject to a simple refactor (field.spanspan).

What's the correct resolution here? Revert refactor, add a test unrelated to the rest of the PR, or ignore?

@pichlermarc
Copy link
Member

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Hmm. This is bothersome. The line missing coverage was subject to a simple refactor (field.spanspan).

What's the correct resolution here? Revert refactor, add a test unrelated to the rest of the PR, or ignore?

Looks like this is just some confusing output from Codecov - it's passing our target of 80%, you you're good :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants